chore(heap-profiling): address heap-profiling feedback [PROF-15190]#2226
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 0ac4b1e | Docs | Datadog PR Page | Give us feedback! |
scottgerring
force-pushed
the
chore/sgg-address-review-feedback
branch
from
dc23ce8 to
16ec8ec
Compare
BenchmarksComparisonBenchmark execution time: 2026-07-14 06:49:43 Comparing candidate commit 0ac4b1e in PR branch Found 0 performance improvements and 10 performance regressions! Performance is the same for 132 metrics, 0 unstable metrics.
|
scottgerring
force-pushed
the
chore/sgg-address-review-feedback
branch
from
16ec8ec to
b7f1281
Compare
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
scottgerring
force-pushed
the
chore/sgg-address-review-feedback
branch
from
fa0ea0a to
9430bef
Compare
scottgerring
force-pushed
the
chore/sgg-address-review-feedback
branch
from
9430bef to
2369b7c
Compare
There was a problem hiding this comment.
Pull request overview
This PR refines the heap-profiling sampler/gotter implementation based on feedback from #2166, focusing on clarifying invariants, reducing allocator-side footguns (size/header drift, errno perturbation), and strengthening integration tests that validate real shipped artifacts.
Changes:
- Added an opt-in Linux
sanity-checkfeature to self-inspect ELF.note.stapsdtand assert exactly one USDT note per expectedddheapprobe. - Centralized x86 sampled-allocation header and bumped-size logic into shared inlines to prevent alloc/free formula drift; renamed
dd_sample_flag_check→dd_sample_flag_check_and_clear. - Improved operational robustness and testability: preserve
errnoaround probes/logging/init, add gotter rescan coordination, and expand install/behavior integration tests (including USDT-note checks).
Reviewed changes
Copilot reviewed 29 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| libdd-profiling-heap-sampler/src/usdt_check.rs | New ELF self-inspection utility for “one note per USDT probe” invariants (opt-in). |
| libdd-profiling-heap-sampler/src/tl_state.c | Preserve errno across first-touch TLS init work. |
| libdd-profiling-heap-sampler/src/sample_flag.c | Align implementation with new header helpers and renamed check API. |
| libdd-profiling-heap-sampler/src/probes.c | Preserve errno across USDT probe emission. |
| libdd-profiling-heap-sampler/src/lib.rs | Export usdt_check behind feature; update test to new check API. |
| libdd-profiling-heap-sampler/src/generated/dd_heap_sampler_static_wrappers.c | Expose new predicate and renamed check for Rust bindings. |
| libdd-profiling-heap-sampler/src/generated/bindings.rs | Bind new dd_alloc_req_is_sampled and renamed dd_sample_flag_check_and_clear. |
| libdd-profiling-heap-sampler/src/allocation_requested.c | Use shared x86 bumped-size helper; preserve errno around sampling/logging. |
| libdd-profiling-heap-sampler/src/allocation_realloc.c | Refactor realloc prepare/commit to use shared header helpers and clearer control flow. |
| libdd-profiling-heap-sampler/src/allocation_freed.c | Use shared bumped-size helper to keep sized-free consistent with alloc side. |
| libdd-profiling-heap-sampler/src/allocation_created.c | Add debug assertion around reentry-guard pairing; improve commentary. |
| libdd-profiling-heap-sampler/include/datadog/heap/sample_flag.h | Add shared x86 size/header helpers; rename check API; consolidate stamping/clearing. |
| libdd-profiling-heap-sampler/include/datadog/heap/allocation_requested.h | Add dd_alloc_req_is_sampled predicate and clarify comments/includes. |
| libdd-profiling-heap-sampler/include/datadog/heap/allocation_freed.h | Update docs and switch to dd_sample_flag_check_and_clear. |
| libdd-profiling-heap-sampler/include/datadog/heap/allocation_created.h | Use the new predicate helper instead of raw weight comparison. |
| libdd-profiling-heap-sampler/docs/tagging.md | Update documentation to match centralized sizing/header logic. |
| libdd-profiling-heap-sampler/Cargo.toml | Add opt-in sanity-check feature and optional deps (elf, anyhow). |
| libdd-profiling-heap-sampler/build.rs | Define NDEBUG for optimized builds to strip C asserts in release. |
| libdd-profiling-heap-gotter/tests/install.rs | Strengthen GOT install tests; add isolation warning and errno passthrough checks. |
| libdd-profiling-heap-gotter/src/lib.rs | Add pending-rescan coordination to avoid missing raced dlopen updates; add test hook-hit accessor. |
| libdd-profiling-heap-gotter/src/hooks.rs | Use new predicate helper; add test hook-hit counter and tweak alignment calc. |
| libdd-profiling-heap-gotter/src/elf.rs | Document a known limitation in patched-library identity detection. |
| libdd-profiling-heap-gotter/Cargo.toml | Add test-support feature for exposing test-only hook-hit counter. |
| libdd-profiling-heap-gotter-ffi/tests/usdt_notes.rs | New integration test validating USDT note uniqueness in the built cdylib. |
| libdd-profiling-heap-gotter-ffi/tests/install.rs | Optionally assert hooks actually ran (via test-support hook-hit counter). |
| libdd-profiling-heap-gotter-ffi/src/lib.rs | Export test-only ddog_heap_gotter_test_hook_hits behind feature gate. |
| libdd-profiling-heap-gotter-ffi/examples/cdylib_demo.rs | Improve demo documentation and safety notes for dlsym transmute. |
| libdd-profiling-heap-gotter-ffi/Cargo.toml | Add test-support feature passthrough and dev-dep enabling sampler sanity-check. |
| libdd-profiling-heap-allocator/src/lib.rs | Clarify live-heap feature documentation. |
| libdd-profiling-heap-allocator/src/allocator.rs | Clarify alignment rationale when freeing sampled allocations. |
| libdd-profiling-heap-allocator/benches/sampler_overhead.rs | Add safety/docs and clarify benchmark intent and constants. |
| Cargo.lock | Add new optional dependencies for sanity-check feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // underlying allocator zeroes everything we hand back. Unsampled | ||
| // path keeps the user's (nmemb, size) verbatim. | ||
| let raw = if req.weight == 0 { | ||
| let raw = if !dd_alloc_req_is_sampled(req) { |
There was a problem hiding this comment.
Yeah, I think this is a real tension. I've inlined it per this with a healthy comment on it to explain why. I've left the C refactoring because it is easier to read.
danielsn
left a comment
danielsn
left a comment
There was a problem hiding this comment.
I have some comments / questions but they're improvements, not things that would block merge.
| fn lock_global_overrides() -> MutexGuard<'static, Option<SymbolOverrides>> { | ||
| GLOBAL_OVERRIDES | ||
| .lock() | ||
| // Recover from poison: hot path never takes this lock, registry re-applies idempotently. |
There was a problem hiding this comment.
Not sure I understand this argument
| // Recover from poison: same rationale as lock_global_overrides. | ||
| Err(TryLockError::Poisoned(poisoned)) => poisoned.into_inner(), | ||
| Err(TryLockError::WouldBlock) => return, | ||
| // Lock held by another install/update: flag a rescan so that thread |
There was a problem hiding this comment.
can there still be a TOCTTOU race here?
There was a problem hiding this comment.
Yeah; i think it's rather unlikely but it could in principle happen, and saying "it resolves when the next library is dlopen'd" is cold comfort if you're loading the last library. I will just serialise the operation with a Mutex.
| // our post-call bookkeeping. A huge aligned_alloc reliably fails with | ||
| // ENOMEM on every libc (size is a multiple of the alignment). | ||
| *libc::__errno_location() = 0; | ||
| let huge = usize::MAX & !0xfff; |
There was a problem hiding this comment.
For some reason, I hadn't noticed that rust uses ! instead of ~ until now
There was a problem hiding this comment.
yeah tbh when I first noticed this it janked me out; I like the distinction between bitwise/logical in C for some reason 🤷
| static inline __attribute__((always_inline)) | ||
| x86_header_t x86_header_read(const void *user) { | ||
| x86_header_t hdr; | ||
| memcpy(&hdr, (const char *)user - DD_HEADER_BYTES, sizeof(hdr)); |
There was a problem hiding this comment.
You could elide the memcpy here with a pointer cast
return *(x86_header_t*)((const char *)user - DD_HEADER_BYTES));There was a problem hiding this comment.
TBH I can see that the memcpy is almost certainly going to always work, but it feels like sailing a bit close to the wind with aliasing concerns. I had a look at the assembler and it gets optimised away anyway - e.g. here's x86 stamp header - its just two movs, one for the header and one for the offset.
movabs rcx, DD_MAGIC
mov qword ptr [rax - 0x10], rcx
mov qword ptr [rax - 0x8], rsi| /* Stamp the header in front of `user` (offset = user - raw). */ | ||
| static inline __attribute__((always_inline)) | ||
| void x86_header_stamp(void *user, uint64_t offset) { | ||
| x86_header_t hdr = { DD_MAGIC, offset }; |
There was a problem hiding this comment.
You could do the same pointer trick I suggest below to make this a one-liner, but not sure that's clearer.
There was a problem hiding this comment.
If we had x86_header_get, this would become
static inline __attribute__((always_inline))
void x86_header_stamp(void *user, uint64_t offset) {
*x86_header_get(user) = (x86_header_t) { DD_MAGIC, offset };
}| const x86_header_t zero = { 0, 0 }; | ||
| memcpy((char *)user - DD_HEADER_BYTES, &zero, sizeof(zero)); |
There was a problem hiding this comment.
or
*x86_header_get(user) = (x86_header_t) { 0,0 };There was a problem hiding this comment.
or even
void x86_header_clear(x86_header_t *hdr) {
*hdr = (x86_header_t) { 0,0 };
}There was a problem hiding this comment.
I like the memset option - it keeps the "we're working with bytes" semantics but makes the intent more explicit.
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
chore/sgg-address-review-feedback
branch
What does this PR do?
Clarifies a bunch of stuff that was not obvious, and improves testing, addressing feedback from @danielsn on #2166
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.